-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes a bug that caused the binary reader not to fail cleanly when parsing incomplete containers in certain cases. #710
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #710 +/- ##
============================================
+ Coverage 67.23% 67.25% +0.01%
- Complexity 5484 5487 +3
============================================
Files 159 159
Lines 23025 23027 +2
Branches 4126 4127 +1
============================================
+ Hits 15481 15486 +5
+ Misses 6262 6259 -3
Partials 1282 1282 ☔ View full report in Codecov by Sentry. |
@@ -1568,6 +1568,9 @@ private boolean uncheckedNextToken() { | |||
if (uncheckedNextContainedToken()) { | |||
return false; | |||
} | |||
if (peekIndex >= limit) { | |||
throw new IonException("Malformed data: declared length exceeds the number of bytes remaining in the stream."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
...exceeds the number of bytes remaining in the container.
be more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to understand why you made this suggestion. It's because we're in an unchecked method, which means that we already believe we have enough data to finish the container. Therefore if peekIndex >= limit
we're definitely past the container bounds because implicitly parent.endIndex < limit
?
Another thing strikes me about this though- we check the same condition peekIndex >= limit
on line 1551, and from my read the only thing that can change that condition in between there and here is this clause of uncheckedNextContainedToken()
:
} else if (parent.typeId.type == IonType.STRUCT) {
if (minorVersion == 0) {
byte b = buffer[(int) peekIndex++];
if (b < 0) {
fieldSid = (b & LOWER_SEVEN_BITS_BITMASK);
} else {
fieldSid = (int) uncheckedReadVarUInt_1_0(b);
}
} else {
uncheckedReadFieldName_1_1();
}
}
We must be in minorVersion == 0
, and I note that uncheckedReadVarUInt_1_0(b)
already contains a malformed data check. Does that mean that this error condition is discovered only when we've just read a 1-byte VarUInt field name, in the if (b < 0) {
case above? In that case, should this error check go there, in uncheckedNextContainedToken()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code appears in the else
branch which handles values below the top level:
if (parent == null) { // Depth 0
// ...
} else {
// the new code
}
I wasn't certain whether limit
was the end of the stream's data or the end of the container's data in the buffer. Based on the location of the code, the latter seemed plausible.
Another thing strikes me about this though- we check the same condition peekIndex >= limit on line 1551, and from my read the only thing that can change that condition in between there and here is this clause of uncheckedNextContainedToken():
I wondered if reset()
might affect peekIndex
or limit
, but didn't investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
...exceeds the number of bytes remaining in the container.
be more accurate?
Yes, this is more accurate.
We must be in minorVersion == 0, and I note that uncheckedReadVarUInt_1_0(b) already contains a malformed data check. Does that mean that this error condition is discovered only when we've just read a 1-byte VarUInt field name, in the if (b < 0) { case above? In that case, should this error check go there, in uncheckedNextContainedToken()?
I preferred to put this check in the proposed location, rather than in uncheckedNextContainedToken()
, because the check protects the line that immediately follows (the access to the buffer at peekIndex
). Note: uncheckedReadVarUInt_1_0
performs the check before each byte it consumes, but the added check protects access to the first byte after the field name.
08af8cb
to
154663e
Compare
…rsing incomplete containers in certain cases.
Issue #, if available:
FasterXML/jackson-dataformats-binary#473
Description of changes:
Before this fix, the added test
expectIncompleteContainerToFailCleanlyAfterFieldSid
would fail withArrayIndexOutOfBoundsException
. The other two tests succeeded before and after the fix; I just wanted to be sure these cases were covered.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.